Skip to content

fix(providers/claude): initialize options.hooks before per-node hook merge#1308

Closed
tharting777 wants to merge 1 commit intocoleam00:devfrom
tharting777:fix/apply-node-config-hooks-init
Closed

fix(providers/claude): initialize options.hooks before per-node hook merge#1308
tharting777 wants to merge 1 commit intocoleam00:devfrom
tharting777:fix/apply-node-config-hooks-init

Conversation

@tharting777
Copy link
Copy Markdown

@tharting777 tharting777 commented Apr 20, 2026

Summary

applyNodeConfig writes into options.hooks[event] without guaranteeing the hooks map is initialized. Any workflow that declares per-node hooks (e.g. archon-architect, archon-refactor-safely) crashes at runtime with:

```
TypeError: undefined is not an object (evaluating 'options.hooks[event] = matchers')
at applyNodeConfig (packages/providers/src/claude/provider.ts:394)
at sendQuery (packages/providers/src/claude/provider.ts:934)
```

The function explicitly acknowledges options.hooks may be undefined one line above (existingHooks typed as SDKHooksMap | undefined), so the fix is to materialize the object before the write loop runs.

Reproduction (pre-fix)

Run any workflow with per-node hooks against the Claude provider — the first AI node crashes in applyNodeConfig. Real-world impact: archon-architect dies on the analyze node; archon-refactor-safely fails identically.

Fix

if (Object.keys(builtHooks).length > 0) {
  const existingHooks = options.hooks as SDKHooksMap | undefined;
  options.hooks ??= {};   // ← added
  for (const [event, matchers] of Object.entries(builtHooks)) { ... }
}

Why this is safe

  • Minimal: one line, landing after the existingHooks capture so the read semantics are preserved.
  • Idempotent when options.hooks exists: ??= is a no-op; the merge-with-existing path runs identically.
  • Fixes the crash path: when options.hooks is undefined, it becomes {}, then the loop populates it — exactly what the original code was trying to do.
  • Downstream contract unchanged: existing tests already use optional chaining (args.options.hooks?.PostToolUse in provider.test.ts:1113), so callers already handle a potentially-absent hooks.

Alternatives considered

  • Initializing options.hooks = {} at the top of applyNodeConfig — changes the contract when nodeConfig.hooks is absent (currently leaves options.hooks untouched).
  • Refactoring the cast-based assignment pattern into typed narrowing — larger scope, better as a follow-up.

Test plan

  • New regression test in provider.test.ts drives sendQuery with nodeConfig.hooks and asserts the SDK receives options.hooks populated. The test fails with the original TypeError on unpatched code (verified by stashing the fix and rerunning).
  • Full suite — bun run test for packages/providers: 69 pass, 0 fail.
  • bun run type-check — all 10 packages pass.
  • bun run lint — clean.
  • bun run format — no changes required.

Happy to adjust wording or expand tests — this surfaced while running archon-architect against an unrelated repo.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced hook configuration initialization to ensure reliable setup and prevent potential undefined access errors when merging node-specific hooks with provider configurations.
  • Tests

    • Added regression test validating that hook configurations are properly initialized and preserved during the merging of node-specific hooks with provider options, even when initial state is undefined.

…merge

applyNodeConfig's merge loop writes into options.hooks without guaranteeing
it is defined. When a workflow declares per-node hooks (e.g. archon-architect,
archon-refactor-safely) and the SDK options arrive with hooks unset, the
cast-based assignment crashes with:

  TypeError: undefined is not an object (evaluating 'options.hooks[event] = matchers')
    at applyNodeConfig (packages/providers/src/claude/provider.ts:394)

The same function acknowledges options.hooks may be undefined one line above
(existingHooks typed as SDKHooksMap | undefined), so the fix is to materialize
the object before the write loop runs.

Add options.hooks ??= {}; after the existingHooks capture — the capture still
references the pre-init value, the merge path's semantics are unchanged when
hooks already exist (no-op), and the else branch that previously crashed now
populates the freshly-initialized map.

Add a regression test that drives sendQuery with a nodeConfig.hooks payload
and asserts the SDK receives options.hooks populated. The test fails with
the original TypeError on the unpatched code.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3bff4b27-6af5-458b-a8f4-9d5926217b66

📥 Commits

Reviewing files that changed from the base of the PR and between 39a05b7 and d180902.

📒 Files selected for processing (2)
  • packages/providers/src/claude/provider.test.ts
  • packages/providers/src/claude/provider.ts

📝 Walkthrough

Walkthrough

The changes add defensive initialization logic to the Claude provider's applyNodeConfig method, ensuring options.hooks is defined before modification. A regression test validates that hooks from nodeConfig are properly merged when the SDK provider's initial hooks are undefined.

Changes

Cohort / File(s) Summary
Hooks initialization safeguard
packages/providers/src/claude/provider.ts
Added options.hooks ??= {} to ensure hooks object is initialized before merging per-node hook structures.
Regression test
packages/providers/src/claude/provider.test.ts
Added test verifying that sendQuery correctly initializes and preserves hook structure when nodeConfig.hooks exists but SDK options.hooks is undefined.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰 A hook here, a hook there,
Options safely initialized with care,
No undefined surprises to fear,
The provider flows crystal clear!
✨🎣

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the primary change: fixing the initialization of options.hooks before the per-node hook merge in the Claude provider.
Description check ✅ Passed The description provides a clear problem statement, root cause analysis, exact fix with code context, safety rationale, test plan with validation results, and alternative approaches considered. It substantially covers required template sections despite not following the exact template structure.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tharting777 tharting777 deleted the fix/apply-node-config-hooks-init branch April 23, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant